-
Notifications
You must be signed in to change notification settings - Fork 651
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Declaring more precise types and purity boundaries on ext-reflection
symbols in .phpstub
files
#8722
Declaring more precise types and purity boundaries on ext-reflection
symbols in .phpstub
files
#8722
Conversation
@kukulich you are the reflection wizard, these days: can you glance over this, if you have time and want to? (not binding, I just think your experience here is extremely valuable) |
Besides feedback provided so far, is this the right way of implementing this patch, with multiple stubs for the same class, each stub "decorating" previous PHP version stubs? If that's the case, then I can gladly apply the provided feedback, get the build green, and then we can move this forward. |
To be honest, this is pretty much uncharted territories, The versioned stubs are somewhat recent (we never needed those before PHP 8) and we mainly built them empirically when issues are raised. So yeah, if it works as is, I, for one, will gladly merge this but I'm not perfectly sure it will. For example, I'm not completely sure we can have two loaded stubs of the same class (especially if there are conflicting methods). If it's causing an issue, we may have to resort to having exclusive stubs loaded for only one PHP version. By the way, sorry to bring this only now, we have an extension folder in which the reflection stubs should probably be extracted (I kinda forgot this since it's something new from Psalm 5 that was added some time ago). If we have to have versioned stubs, this will be easier to have a Reflection71, Reflection72... in there |
Note: many reflection-related suppressions depend on vimeo/psalm#8722 Also, we preserved many assertions on `class-string`, since removing input validation would be a BC break.
…` symbols in `.phpstub` files Also: * added PHP 8.2 stubs * refined types to make impossible scenarios more clear (like `ReflectionIntersectionType#allowsNull()`) This is a first attempt at refining these types: the structure of these stubs is quite confusing to me, so I don't know if this approach is correct, and if the stubs are merged together, or if entire symbols need to be completely re-declared for each PHP version.
…-string` Because @weirdan is a party pooper (they poop at the parties) Ref: https://www.youtube.com/watch?v=gjwofYhUJEM Ref: vimeo#8722 (comment)
e4eb00b
to
d5cccba
Compare
As per @weirdan's feedback, we can prevent the usage of `object` instances that implement `__invoke()`, as well as `array` callables, by declaring the ctor argument of `ReflectionFunction` to be either a real `Closure`, or a `callable-string`. While this may not be 100% of scenarios, it is a healthy way to identify errors in userland. Ref: vimeo#8722 (comment)
These templates were leading to false positives: assuming an `object` is given as input, the inferred return type would always have been `true`, which is obviously not valid. Removing them is the healthier alternative, for now. Ref: vimeo#8722 (comment)
I did some more local testing: PHP 8.2 stubs are somehow not picked up correctly. That will take some time to debug :D |
I did some digging to make sure that the stubs work as expected. Here's what I did to validate this patch locally (since I don't think it can really be fully automated) TL;DR: ✔️ works as expected in my own test expectations Create a dummy file to verify used symbols<?php
namespace Testing;
/** @return \ReflectionClass<\stdClass> */
function getAClass(): \ReflectionClass
{
throw new \Exception('irrelevant');
}
function getAnUnionType(): \ReflectionUnionType
{
throw new \Exception('irrelevant');
}
function getAnIntersectionType(): \ReflectionIntersectionType
{
throw new \Exception('irrelevant');
}
function getAProperty(): \ReflectionProperty
{
throw new \Exception('irrelevant');
}
function getAMethod(): \ReflectionMethod
{
throw new \Exception('irrelevant');
}
// verifying that `getName()` is stubbed in all versions: this should always be a `class-string<\stdClass>`
$name = getAClass()->getName();
// union types should appear starting with PHP 8.0
$unionTypes = getAnUnionType()->getTypes();
// intersection types should appear starting with PHP 8.1, and change slightly with PHP 8.2
$intersectionTypes = getAnIntersectionType()->getTypes();
getAProperty()->setAccessible(true); // should be marked as unused expression starting from PHP 8.1
getAMethod()->setAccessible(true); // should be marked as unused expression starting from PHP 8.1
$results = [$name, $unionTypes, $intersectionTypes, \ReflectionMethod::IS_PUBLIC];
/** @psalm-trace $results */ // tracing this will show us the differences between versions
return $results; Run the script against various
|
… 8.1 and PHP 8.2 * in PHP 8.0, `ReflectionUnionType` is composed on `ReflectionNamedType`s * in PHP 8.1, `ReflectionIntersectionType` is composed of `ReflectionNamedType`s * in PHP 8.2, `ReflectionUnionType` is composed of `ReflectionIntersectionType|ReflectionNamedType`s Slight variations for each PHP version. As per local testing, this doesn't work yet. ## Local testing setup: I did some digging to make sure that the stubs work as expected. Here's what I did to validate this patch locally (since I don't think it can really be fully automated) ## Create a dummy file to verify used symbols ```php <?php namespace Testing; /** @return \ReflectionClass<\stdClass> */ function getAClass(): \ReflectionClass { throw new \Exception('irrelevant'); } function getAnUnionType(): \ReflectionUnionType { throw new \Exception('irrelevant'); } function getAnIntersectionType(): \ReflectionIntersectionType { throw new \Exception('irrelevant'); } // verifying that `getName()` is stubbed in all versions: this should always be a `class-string<\stdClass>` $name = getAClass()->getName(); // union types should appear starting with PHP 8.0. Starting with PHP 8.2, they allow for intersections. $unionTypes = getAnUnionType()->getTypes(); // intersection types should appear starting with PHP 8.1 $intersectionTypes = getAnIntersectionType()->getTypes(); $results = [$name, $unionTypes, $intersectionTypes]; /** @psalm-trace $results */ // tracing this will show us the differences between versions return $results; ``` ## Run the script against various `vimeo/psalm` versions ```sh docker run --rm -ti -v $(pwd):/app -w /app php:7.4 ./psalm --php-version=7.4 --no-cache reflection-test.php | grep Trace docker run --rm -ti -v $(pwd):/app -w /app php:8.0 ./psalm --php-version=8.0 --no-cache reflection-test.php | grep Trace docker run --rm -ti -v $(pwd):/app -w /app php:8.1 ./psalm --php-version=8.1 --no-cache reflection-test.php | grep Trace docker run --rm -ti -v $(pwd):/app -w /app php:8.2.0RC7-cli ./psalm --php-version=8.2 --no-cache reflection-test.php | grep Trace ``` ## Evaluate output ``` ❯ docker run --rm -ti -v $(pwd):/app -w /app php:7.4 ./psalm --php-version=7.4 --no-cache reflection-test.php | grep Trace ERROR: Trace - reflection-test.php:20:1 - $results: list{class-string<stdClass>, mixed, mixed} (see https://psalm.dev/224) ❯ docker run --rm -ti -v $(pwd):/app -w /app php:8.0 ./psalm --php-version=8.0 --no-cache reflection-test.php | grep Trace ERROR: Trace - reflection-test.php:20:1 - $results: list{class-string<stdClass>, non-empty-list<ReflectionNamedType>, mixed} (see https://psalm.dev/224) ❯ docker run --rm -ti -v $(pwd):/app -w /app php:8.1 ./psalm --php-version=8.1 --no-cache reflection-test.php | grep Trace ERROR: Trace - reflection-test.php:20:1 - $results: list{class-string<stdClass>, non-empty-list<ReflectionNamedType>, non-empty-list<ReflectionNamedType>} (see https://psalm.dev/224) psalm on feature/vimeo#8720-improve-types-and-purity-for-reflection-symbols [!?] via 🐘 v8.1.13 via ❄️ impure (nix-shell) took 4s ❯ docker run --rm -ti -v $(pwd):/app -w /app php:8.2.0RC7-cli ./psalm --php-version=8.2 --no-cache reflection-test.php | grep Trace ERROR: Trace - reflection-test.php:20:1 - $results: list{class-string<stdClass>, non-empty-list<ReflectionNamedType>, non-empty-list<ReflectionNamedType>} (see https://psalm.dev/224) ```
…rom PHP 8.1 onwards This will highlight unused code. Ref: php/php-src#5412 Ref: https://wiki.php.net/rfc/make-reflection-setaccessible-no-op Ref: php/php-src#5411 Example https://3v4l.org/PNeeZ ```php <?php class Foo { private $bar = 'baz'; private function taz() { return 'waz'; } } //var_dump((new ReflectionProperty(Foo::class, 'bar'))->getValue(new Foo)); //var_dump((new ReflectionMethod(Foo::class, 'taz'))->invoke(new Foo)); ``` Produces (starting from PHP 8.1): ``` string(3) "baz" string(3) "waz" ```
…come in a `list`
Before this change, preloaded stubs would only be loaded when running on a PHP version lower than the one that is in the stubs. With this change, the analysis PHP version (defined via config, input parameter, or inferred from the runtime) becomes authoritative. Since the PHP-version-specific stubs are not just polyfills, but actually type refinements on top of the PHP core symbols at hand, this change always loads them, so that it is possible to get more precise type inference downstream. This will likely lead to downstream breakages, because the stubs do indeed provide better type resolution, but indeed formalizes the idea that these stubs will provide better value for finding problems in analyzed code.
Thanks for your work on this! I'll find some time this week end to look at what's going on to see if I can help :) |
This massively reduces the internal complexity of the `TypeGenerator`, since we convert `ReflectionType` symbols **directly** to our `@internal` data structures, ready to be rendered. Before this, we would cast the whole `ReflectionType` to a `string` that fits our need, and then we would go back to parsing that string, with a substantial overhead (especially considering the newly introduced validation rules around DNF types). Note that this introduces new Psalm violations that were added to `psalm-baseline.xml`, but which are solved by my work @ vimeo/psalm#8722 Signed-off-by: Marco Pivetta <ocramius@gmail.com>
…ly when visiting stub files While `visitPreloadedStubFiles` seemed to work at first, it led to overriding symbols declared by PHP itself too eagerly. By only loading PHP-version-specific stubs in `visitStubFiles`, we ensure that the reflection-declared symbols take priority, and that our stubs overlay on top of that, without actually replacing the symbol entirely, but rather merging with its definition. This fixes current test failures too, and works with the code examples from vimeo#8722 (comment)
@orklah I think actual coding work on this is complete: what is missing is validation / decision making, but otherwise we are green :) Hopefully that means not cutting too much into your time anymore 👍 |
Thanks Marco! I checked that and I was surprised that Stringable was not flagged as an error here: https://psalm.dev/r/3d60a7e0ea?phpVersion=7.1 So we already have global namespace pollution and it didn't cause much issue, I'm not worried about Reflection classes leaking then. I'd be interested in the result of running Psalm against BetterReflection in different php versions for verification but otherwise, I'm good! We can merge when ready :) |
I found these snippets: https://psalm.dev/r/3d60a7e0ea<?php
class A implements Stringable{}
|
The two most aggressive reflection codebases I know of are:
I will check both against this patch and report back 👍 |
I ran a report over laminas-laminas-code-report.txt roave-betterreflection-report.txt Overall just |
Seems great! Thanks a lot Marco! |
🎉 |
As per PHP 8.1, this call has no effect anymore. Ref: vimeo/psalm#8722 Ref: https://wiki.php.net/rfc/make-reflection-setaccessible-no-op
Also:
ReflectionIntersectionType#allowsNull()
)This is a first attempt at refining these types: the structure of these stubs is quite confusing to me, so I don't know if this approach is correct, and if the stubs are merged together, or if entire symbols need to be completely re-declared for each PHP version.
Fixes #8720
Fixes #6378